fix(ffi): free transaction byte buffers correctly by thepastaclaw · Pull Request #749 · dashpay/rust-dashcore

thepastaclaw · 2026-05-09T02:10:38Z

Fix transaction byte buffer freeing

Summary

Preserve the existing one-argument transaction_bytes_free C ABI.
Store the returned transaction byte length in a hidden prefix so the Rust free
function can reconstruct and drop the original boxed slice layout safely.
Route both transaction byte producers through the same allocation helper.
Add unit coverage for null, empty, and non-empty buffers.

Validation

cargo test -p key-wallet-ffi transaction_bytes_free --lib
python3 contrib/verify_ffi.py
git diff --check
npx markdownlint-cli2 /tmp/pr-body.md
Code review gate passed with recommendation: ship.

coderabbitai · 2026-05-09T02:10:45Z

Warning

Rate limit exceeded

@thepastaclaw has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 51 minutes and 39 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8cb0aca6-3111-433e-ad0d-1d8f49380f47

📥 Commits

Reviewing files that changed from the base of the PR and between e9296b2 and a7d48fb.

📒 Files selected for processing (3)

key-wallet-ffi/FFI_API.md
key-wallet-ffi/src/transaction.rs
key-wallet-ffi/src/transaction_tests.rs

📝 Walkthrough

Walkthrough

This PR refactors FFI transaction buffer memory management to use a hidden length prefix. Transaction serialization now allocates a buffer prefixed with a usize length field, returns a pointer past the prefix to callers, and deallocates by reading the stored prefix to reconstruct the full allocation. Documentation and tests validate the new contract.

Changes

FFI Transaction Buffer Memory Management

Layer / File(s)	Summary
Buffer Layout & Allocation `key-wallet-ffi/src/transaction.rs`	New `TRANSACTION_BYTES_LEN_PREFIX_SIZE` constant and `transaction_bytes_into_ffi_buffer` helper allocate a buffer with a hidden `usize` length prefix and return a pointer to the transaction bytes region.
Memory Deallocation `key-wallet-ffi/src/transaction.rs`	`transaction_bytes_free` is updated to read the length prefix from memory before the caller-provided pointer, reconstruct the original boxed allocation, and safely deallocate.
Transaction Build Integration `key-wallet-ffi/src/transaction.rs`	Both `wallet_build_and_sign_transaction` and `wallet_build_and_sign_asset_lock_transaction` are updated to use `transaction_bytes_into_ffi_buffer` for serialization and output, with Rust documentation clarifying the `transaction_bytes_free` freeing requirement.
FFI API Documentation `key-wallet-ffi/FFI_API.md`	Documentation for `transaction_bytes_free` is refined to specify pointer validity constraints, and both build functions' documentation is expanded to explicitly state the freeing requirement via `transaction_bytes_free(*tx_bytes_out)`.
Buffer Lifecycle Tests `key-wallet-ffi/src/transaction_tests.rs`	New test module verifies null-pointer safety, successful deallocation of prefixed buffers, and correct length semantics for empty payloads.

🎯 3 (Moderate) | ⏱️ ~25 minutes

A rabbit hops through memory,
prefix in hand, pointer unbent,
freed without a double-take—
allocation dreams, fully spent. 🐰✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix(ffi): free transaction byte buffers correctly' directly summarizes the main change: fixing incorrect buffer freeing behavior for FFI transaction bytes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

thepastaclaw · 2026-05-09T02:12:04Z

@coderabbitai review

coderabbitai · 2026-05-09T02:12:10Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@key-wallet-ffi/FFI_API.md`:
- Around line 1308-1311: The docs for the build-and-sign transaction function
refer to a parameter named `fee_rate` but the actual function signature uses
`fee_per_kb`; update all occurrences of `fee_rate` in the function description
and the Safety section to `fee_per_kb` (including the parameter list and
explanatory text) so the documentation matches the FFI signature, then
regenerate FFI_API.md to pick up the corrected source docs; reference the
documented function's parameter `fee_per_kb` and the Safety section entries for
`fee_rate` to locate and replace the mismatched names.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 32e6f687-4825-4656-87a3-c169bd9cad5c

📥 Commits

Reviewing files that changed from the base of the PR and between 34f574d and e9296b2.

📒 Files selected for processing (3)

key-wallet-ffi/FFI_API.md
key-wallet-ffi/src/transaction.rs
key-wallet-ffi/src/transaction_tests.rs

codecov · 2026-05-09T02:19:53Z

Codecov Report

❌ Patch coverage is 54.16667% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.91%. Comparing base (34f574d) to head (a7d48fb).
⚠️ Report is 26 commits behind head on dev.

Files with missing lines	Patch %	Lines
key-wallet-ffi/src/transaction.rs	54.16%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #749      +/-   ##
==========================================
+ Coverage   71.89%   71.91%   +0.02%     
==========================================
  Files         320      320              
  Lines       69473    69465       -8     
==========================================
+ Hits        49947    49957      +10     
+ Misses      19526    19508      -18

Flag	Coverage Δ
core	`76.30% <ø> (ø)`
ffi	`46.57% <54.16%> (+0.07%)`	⬆️
rpc	`20.00% <ø> (ø)`
spv	`87.88% <ø> (+0.03%)`	⬆️
wallet	`71.22% <ø> (+0.03%)`	⬆️

Files with missing lines	Coverage Δ
key-wallet-ffi/src/transaction.rs	`2.92% <54.16%> (+2.92%)`	⬆️

... and 3 files with indirect coverage changes

Transaction-building FFI functions (`wallet_build_and_sign_transaction`, `wallet_build_and_sign_asset_lock_transaction`) hand back the serialized transaction as `Vec<u8>` -> `Box<[u8]>` -> `Box::into_raw(...) as *mut u8`. The previous `transaction_bytes_free(*mut u8)` then reconstructed `Box<u8>`, which is the wrong allocation layout for a boxed slice and is undefined behavior. Take the original length back from the caller and reconstruct the boxed slice via `Box::from_raw(slice_from_raw_parts_mut(ptr, len))`. Null remains a no-op. Update the safety docs on both builders and on `transaction_bytes_free` itself, refresh `FFI_API.md`, and add focused unit tests covering null free, freeing a `Box<[u8]>` with the matching length, and the empty-slice case. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

thepastaclaw · 2026-05-21T12:39:58Z

@coderabbitai review

coderabbitai · 2026-05-21T12:40:06Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

thepastaclaw · 2026-05-21T12:48:12Z

Checked the current head after the CodeRabbit retry: key-wallet-ffi/FFI_API.md and the source docs now use fee_per_kb for wallet_build_and_sign_transaction; I don’t see any remaining fee_rate references for that API. @coderabbitai review

coderabbitai · 2026-05-21T12:48:26Z

@thepastaclaw Thanks for confirming — good to know the fee_rate → fee_per_kb rename is fully propagated across FFI_API.md and the source docs. Triggering a fresh review now.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

thepastaclaw force-pushed the tracker-1220 branch from e9296b2 to 4b4f9f9 Compare May 9, 2026 02:14

coderabbitai Bot requested changes May 9, 2026

View reviewed changes

Comment thread key-wallet-ffi/FFI_API.md Outdated

thepastaclaw force-pushed the tracker-1220 branch from 4b4f9f9 to a7d48fb Compare May 9, 2026 02:50

thepastaclaw mentioned this pull request May 9, 2026

dash-spv: non-exclusive peer disconnection test times out on Ubuntu ARM #750

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ffi): free transaction byte buffers correctly#749

fix(ffi): free transaction byte buffers correctly#749
thepastaclaw wants to merge 1 commit into
dashpay:devfrom
thepastaclaw:tracker-1220

thepastaclaw commented May 9, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented May 9, 2026 •

edited

Loading

Rate limit exceeded

Walkthrough

Changes

Uh oh!

thepastaclaw commented May 9, 2026

Uh oh!

coderabbitai Bot commented May 9, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

codecov Bot commented May 9, 2026 •

edited

Loading

Uh oh!

thepastaclaw commented May 21, 2026

Uh oh!

coderabbitai Bot commented May 21, 2026

Uh oh!

thepastaclaw commented May 21, 2026

Uh oh!

coderabbitai Bot commented May 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

thepastaclaw commented May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix transaction byte buffer freeing

Summary

Validation

Uh oh!

coderabbitai Bot commented May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Walkthrough

Changes

Uh oh!

thepastaclaw commented May 9, 2026

Uh oh!

coderabbitai Bot commented May 9, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thepastaclaw commented May 21, 2026

Uh oh!

coderabbitai Bot commented May 21, 2026

Uh oh!

thepastaclaw commented May 21, 2026

Uh oh!

coderabbitai Bot commented May 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

thepastaclaw commented May 9, 2026 •

edited

Loading

coderabbitai Bot commented May 9, 2026 •

edited

Loading

codecov Bot commented May 9, 2026 •

edited

Loading